Instrumenter API improvements #2860
Conversation
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
pavolloffay,
trask and
tylerbenson
as code owners
.../main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanNameExtractor.java
Show resolved
Hide resolved
| * network peer information. If those attributes are available in the request, it is recommended to | ||
| * use {@link InetSocketAddressNetAttributesExtractor} instead. | ||
| */ | ||
| public abstract class InetSocketAddressNetRequestAttributesExtractor<REQUEST, RESPONSE> |
There was a problem hiding this comment.
Didn't realize this would be needed already :)
I have considered an approach where there is only one extractor, and @Nullable RESPONSE response - if it's available when response is null (onStart), then it's gotten from request, otherwise gotten when response isn't null (onEnd). The advantage of two classes is it's easier to implement them. The disadvantage is having cognitive load deciding which class to use. I think I'm leaning towards the separate classes actually but wanted to get your thoughts too.
| @@ -26,10 +31,6 @@ protected final void onStart(AttributesBuilder attributes, REQUEST request) { | |||
| @Override | |||
| protected final void onEnd(AttributesBuilder attributes, REQUEST request, RESPONSE response) { | |||
| set(attributes, SemanticAttributes.NET_PEER_IP, peerIp(request, response)); | |||
|
|
|||
| // TODO(anuraaga): Clients don't have peer information available during the request usually. | |||
There was a problem hiding this comment.
But related to my above comment, actually this isn't quite solved since it means instrumentation defining separate ServerNetAttributesExtractor and ClientNetAttributesExtractor, which our pattern doesn't really support since it makes instrumentation more complex. The approach with @Nullable RESPONSE response could solve it though.
There was a problem hiding this comment.
I've refactored the NetAttributesExtractor class so that it has a @Nullable response in its attribute methods.
* Move HTTP & net classes to separate packages * Remove `db` prefix from method names in `DbAttributesExtractor` * Add request-only net attributes extractor (it'll be needed in spring-sleuth-otel once we decide to use Instrumenters there)
mateuszrzeszutek
force-pushed
the
refactor-existing-instrumenter-api-parts
branch
from
c729d58 to
6e8ddcf
Compare
| String route = attributesExtractor.route(request); | ||
| if (route != null) { | ||
| return route; | ||
| } |
...ain/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanStatusExtractor.java
Outdated
Show resolved
Hide resolved
...t/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanNameExtractorTest.java
Outdated
Show resolved
Hide resolved
...java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanStatusExtractorTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Trask Stalnaker <[email protected]>
dbprefix from method names inDbAttributesExtractor